refactor: migrate ref methods to command #3953

YangJonghun · 2024-06-29T17:41:34Z

Summary

migrate ref methods to command
add deprecated warning into getReactTag function
remove unused native methods (presentFullscreenPlayer, dismissFullscreenPlayer)
wrong type change (drm.getLicense)
- as-is: () => void
- to-be: () => string | Promise<string>

Motivation

we need to remove findNodeHandle (link)
Command doesn't allow return value or callback argument. however save and getCurrentPosition functions return a value. so we have to consider about whether to keep it or drop it.

Changes

Test plan

freeboub · 2024-06-30T15:25:17Z

src/types/ViewType.ts

@@ -2,10 +2,10 @@
 * Define Available view type for android
 * these values shall match android spec, see ViewType.kt
 */
-enum ResizeMode {


android/src/main/java/com/brentvatne/exoplayer/ReactExoplayerViewManager.java

src/specs/VideoNativeComponent.ts

KrzysztofMoch · 2024-07-01T14:50:39Z

I will check if Interop Layer will work with those changes and I am fine to merge this (after @freeboub review)

src/specs/NativeVideoDecoderProperties.ts

src/specs/NativeVideoManagerModule.ts

src/specs/VideoNativeComponent.ts

KrzysztofMoch · 2024-07-02T07:47:20Z

@YangJonghun It also seems that we need to do some renaming as setVolume as Command collides with some other setVolume (probably from volume prop) - I suggest to rename setVolume Command to setVolumeCMD. It will be only internal change as we wrap commands in out component

KrzysztofMoch · 2024-07-02T09:06:31Z

I am also wandering if we should use TurboModuleRegistry.getEnforcing with old architecture "package" (Turbo modules has it own way to export modules in package - you can find it here). I have check it and we cannot move to it (new TurboModules package way) yet as we need to firstly move view to fabric.

All those changes that I am required from you are from my process of trying to build on New Arch - And I think that we cannot break support for Interop Layer as developers can depend on it. So for now it would be better to back to requireNativeComponent as with those changes I am not able to run app in new arch anymore.
WDYT @YangJonghun @freeboub ?

YangJonghun · 2024-07-02T12:25:11Z

@KrzysztofMoch
Thanks for comment!
But AFAIK TurboModuleRegistry.getEnforcing works properly on interop layer probably because we didn't set congenConfig.
Also I confirmed that it runs on version 0.74 with new architecture.

YangJonghun · 2024-07-02T12:30:51Z

@YangJonghun It also seems that we need to do some renaming as setVolume as Command collides with some other setVolume (probably from volume prop) - I suggest to rename setVolume Command to setVolumeCMD. It will be only internal change as we wrap commands in out component

I agree, but I'm a little confused, Am I correct in asking for your suggestion like code below?

    const setVolume = useCallback((volume: number) => {
-      return nativeRef.current && Commands.setVolume(nativeRef.current, volume);
+     return nativeRef.current && Commands.setVolumeCMD(nativeRef.current, volume);
    }, []);

KrzysztofMoch · 2024-07-02T12:38:48Z

Also I confirmed that it runs on version 0.74 with new architecture.

👀 In our example ? I have tried to run app with react-native-video (built from this PR) but I wasn't able to do it in basic example nor "clean" RN app (in both I used [email protected])

Am I correct in asking for your suggestion like code below?

Yes this is what I meant

YangJonghun · 2024-07-02T12:46:22Z

👀 In our example ? I have tried to run app with react-native-video (built from this PR) but I wasn't able to do it in basic example nor "clean" RN app (in both I used [email protected])

No, I tested on my own bare react-native project (created via npx react-native init)
I tried with example! Thanks for let me know

KrzysztofMoch · 2024-07-02T12:59:47Z

No, I tested on my own bare react-native project (created via npx react-native init) I tried with example! Thanks to let me know

Strange I also tried to run it on new bare react-native project - it built but there was an error that VideoManager Module is missing "in TurboModule list" (as if it was not detected)

YangJonghun · 2024-07-02T13:48:41Z

@KrzysztofMoch
I confirmed error you mentioned. Thanks
I'll be restore NativeModules

YangJonghun · 2024-07-02T14:29:10Z

@KrzysztofMoch
If we need to support interop layer, I think we need to drop this PR.
New arch doesn't support partial migration.🥲
or create new stacked branch and merge it for prepare to support new arch.

KrzysztofMoch · 2024-07-02T14:32:11Z

This is something that also thought. I think it would be better to create new arch branch - then step by step migrate it
@freeboub WDYT?

freeboub · 2024-07-02T20:26:52Z

android/src/main/java/com/brentvatne/react/VideoDecoderPropertiesModule.kt

@@ -67,6 +67,6 @@ class VideoDecoderPropertiesModule(reactContext: ReactApplicationContext?) : Rea
    companion object {
        private val WIDEVINE_UUID = UUID(-0x121074568629b532L, -0x5c37d8232ae2de13L)
        private const val SECURITY_LEVEL_PROPERTY = "securityLevel"
-        private const val REACT_CLASS = "VideoDecoderProperties"


is it really necessary to change the module name ?

No, it's not required, but I changed it to reduce the chance of duplicates with other library(or core).

freeboub · 2024-07-02T20:28:28Z

ios/Video/RCTDecoderPropertiesModule.m

+
+RCT_EXPORT_MODULE(RNVDecoderPropertiesModule);
+
+RCT_EXPORT_METHOD(getWidevineLevel : (RCTPromiseResolveBlock)resolve reject : (RCTPromiseRejectBlock)reject) {}


I think you should al least resolve or reject the Promise... (maybe reject with an error "not implemented") , for these 3 methods

These functions are meaningless but, acceptable.
I think we should throw an error to notify library contributors.
(we should wrapping decoder properties module on JS side)

freeboub · 2024-07-02T20:32:49Z

src/Video.tsx

@@ -615,15 +611,14 @@ const Video = forwardRef<VideoRef, ReactVideoProps>(
          drm={_drm}
          style={StyleSheet.absoluteFill}
          resizeMode={resizeMode}
-          fullscreen={isFullscreen}


This PR is big, can you confirm the fullscreen property is still available for apps ?

previously, we use internal fullscreen state(for handle present,dismiss fullscreen command) but it was meaningless.
fullscreen prop will be inject with {...rest} and fullscreen commands are work with command.setFullscreen(value).
previous
Rather, the old code was causing different behavior than expected. when user changing fullscreen prop value.

freeboub · 2024-07-02T20:36:08Z

This is something that also thought. I think it would be better to create new arch branch - then step by step migrate it @freeboub WDYT?

Yes It can make sense, but I am really afraid of divergencies... I don't think this PR is too much risky ... but it is still better to merge steps by steps...

YangJonghun · 2024-07-08T11:44:52Z

@freeboub @KrzysztofMoch
Moved some code fixes to PR #3980

YangJonghun added 6 commits June 30, 2024 02:18

chore: add warning

c47c54f

refactor: fix type

ae89255

refactor(native): migrate module to command

cd44e46

chore(ios): fix lint error

ed3c3cf

Merge branch 'master' into refactor/migrate-to-command

c620ba1

chore: fix enum name

c7533be

freeboub reviewed Jun 30, 2024

View reviewed changes

Merge branch 'master' into refactor/migrate-to-command

57cf079

YangJonghun force-pushed the refactor/migrate-to-command branch from 81ff08d to 57cf079 Compare June 30, 2024 17:02

KrzysztofMoch requested changes Jul 1, 2024

View reviewed changes

android/src/main/java/com/brentvatne/exoplayer/ReactExoplayerViewManager.java Outdated Show resolved Hide resolved

src/specs/VideoNativeComponent.ts Outdated Show resolved Hide resolved

YangJonghun added 2 commits July 1, 2024 23:44

fix(android): fix setFullScreen command value

6bfb209

refactor: remove repeated type definition

0a6c216

KrzysztofMoch requested changes Jul 1, 2024

View reviewed changes

src/specs/NativeVideoDecoderProperties.ts Outdated Show resolved Hide resolved

src/specs/NativeVideoManagerModule.ts Outdated Show resolved Hide resolved

src/specs/VideoNativeComponent.ts Outdated Show resolved Hide resolved

fix(type): fix type for codegen

be60e18

YangJonghun requested a review from KrzysztofMoch July 1, 2024 18:50

refactor: rename of cmd.setVolume

dd52509

chore: fix lint error

5f14756

freeboub reviewed Jul 2, 2024

View reviewed changes

Merge branch 'master' into refactor/migrate-to-command

a3548fe

KrzysztofMoch added on-hold PR is on hold for some important reason new architecture PR that contain migration to new architecture labels Jul 4, 2024

Merge branch 'master' into refactor/migrate-to-command

c7d29ff

YangJonghun closed this Jul 12, 2024

YangJonghun deleted the refactor/migrate-to-command branch July 12, 2024 17:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: migrate ref methods to command #3953

refactor: migrate ref methods to command #3953

YangJonghun commented Jun 29, 2024 •

edited

Loading

freeboub Jun 30, 2024

KrzysztofMoch commented Jul 1, 2024

KrzysztofMoch commented Jul 2, 2024

KrzysztofMoch commented Jul 2, 2024

YangJonghun commented Jul 2, 2024

YangJonghun commented Jul 2, 2024

KrzysztofMoch commented Jul 2, 2024 •

edited

Loading

YangJonghun commented Jul 2, 2024 •

edited

Loading

KrzysztofMoch commented Jul 2, 2024

YangJonghun commented Jul 2, 2024

YangJonghun commented Jul 2, 2024

KrzysztofMoch commented Jul 2, 2024

freeboub Jul 2, 2024

YangJonghun Jul 3, 2024 •

edited

Loading

freeboub Jul 2, 2024 •

edited

Loading

YangJonghun Jul 3, 2024

freeboub Jul 2, 2024

YangJonghun Jul 3, 2024

freeboub commented Jul 2, 2024

YangJonghun commented Jul 8, 2024


		RCT_EXPORT_MODULE(RNVDecoderPropertiesModule);

		RCT_EXPORT_METHOD(getWidevineLevel : (RCTPromiseResolveBlock)resolve reject : (RCTPromiseRejectBlock)reject) {}

refactor: migrate ref methods to command #3953

refactor: migrate ref methods to command #3953

Conversation

YangJonghun commented Jun 29, 2024 • edited Loading

Summary

Motivation

Changes

Test plan

freeboub Jun 30, 2024

Choose a reason for hiding this comment

KrzysztofMoch commented Jul 1, 2024

KrzysztofMoch commented Jul 2, 2024

KrzysztofMoch commented Jul 2, 2024

YangJonghun commented Jul 2, 2024

YangJonghun commented Jul 2, 2024

KrzysztofMoch commented Jul 2, 2024 • edited Loading

YangJonghun commented Jul 2, 2024 • edited Loading

KrzysztofMoch commented Jul 2, 2024

YangJonghun commented Jul 2, 2024

YangJonghun commented Jul 2, 2024

KrzysztofMoch commented Jul 2, 2024

freeboub Jul 2, 2024

Choose a reason for hiding this comment

YangJonghun Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

freeboub Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

YangJonghun Jul 3, 2024

Choose a reason for hiding this comment

freeboub Jul 2, 2024

Choose a reason for hiding this comment

YangJonghun Jul 3, 2024

Choose a reason for hiding this comment

freeboub commented Jul 2, 2024

YangJonghun commented Jul 8, 2024

YangJonghun commented Jun 29, 2024 •

edited

Loading

KrzysztofMoch commented Jul 2, 2024 •

edited

Loading

YangJonghun commented Jul 2, 2024 •

edited

Loading

YangJonghun Jul 3, 2024 •

edited

Loading

freeboub Jul 2, 2024 •

edited

Loading